Skip to content

Correctly type check Class<T>#1698

Open
HT154 wants to merge 1 commit into
apple:mainfrom
HT154:class-no-erasure
Open

Correctly type check Class<T>#1698
HT154 wants to merge 1 commit into
apple:mainfrom
HT154:class-no-erasure

Conversation

@HT154

@HT154 HT154 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

This PR removes the unconditional type-erasure of Class<T> to Class.

This is a breaking change when T is a union type, nullable type, string literal type, parameterized type, type alias, or nothing; these are now error conditions.

For convenience, erasure still occurs when T is unknown or Any. Similarly, the subclass check is skipped when T is an un-replaced type variable, e.g. in generic classes/methods (type aliases replace the type arg during instantiation).

Resolves #1729
Closes apple/pkl-intellij#222

@HT154 HT154 force-pushed the class-no-erasure branch 9 times, most recently from 7c64e69 to 5adcbf7 Compare June 27, 2026 20:26
@HT154 HT154 force-pushed the class-no-erasure branch 2 times, most recently from c2f4029 to d31129f Compare June 27, 2026 21:33
@HT154 HT154 force-pushed the class-no-erasure branch from d31129f to 20e15db Compare June 28, 2026 22:44
@HT154 HT154 force-pushed the class-no-erasure branch 4 times, most recently from 17b645c to bfb8210 Compare June 30, 2026 19:36
@HT154 HT154 marked this pull request as ready for review June 30, 2026 22:33
@HT154 HT154 force-pushed the class-no-erasure branch 4 times, most recently from 096cb0d to acd6e0a Compare July 2, 2026 22:12
@bioball

bioball commented Jul 2, 2026

Copy link
Copy Markdown
Member

General comment: given this:

typealias MyNumber = Number
foo: Class<MyNumber> = Number

This seems like an incorrect error, because MyNumber is just an alias for Number.

–– Pkl Error ––
`Class` type arguments may only be class types, module types, `unknown`, or `module`.

9 | foo: Class<MyNumber> = Number
         ^^^^^^^^^^^^^^^
at test (/Users/danielchao/code/apple/pkl/.dan-scripts/test.pkl:9)

Might also be okay to not throw for type arguments which don't point to classes. In these cases, the type would just never typecheck, and it would behave just like the nothing type. For example:

a: Class<Foo?>
b: Class<"foo" | "bar>

@HT154

HT154 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

General comment: given this:

typealias MyNumber = Number
foo: Class<MyNumber> = Number

This seems like an incorrect error, because MyNumber is just an alias for Number.

Can you say more about how this would be useful? The only case I can think of is the compatibility shim typealias when a class is moved out into its own module.

If this was supported, it would only work for aliases to bare class types, which is of limited utility. An alias may hide that type is not an ordinary class and result in an error (as-is) or a nothing type (as you suggest below), especially if a library author later refactors an alias to include a constraint where one did not exist previously.

Might also be okay to not throw for type arguments which don't point to classes. In these cases, the type would just never typecheck, and it would behave just like the nothing type. For example:

a: Class<Foo?>
b: Class<"foo" | "bar>

Again, I think it's more surprising that users could inadvertently build a nothing type without realizing and have all their type checks silently fail. This would still be a breaking change compared to the existing behavior (where these are essentially erased to unknown) and I'd prefer to break in the direction of providing helpful user feedback than silent and potentially-surprising behavior.

@bioball

bioball commented Jul 2, 2026

Copy link
Copy Markdown
Member

Can you say more about how this would be useful? The only case I can think of is the compatibility shim typealias when a class is moved out into its own module.

An invariant of a typealias is that: on the type level, it just stands for its aliased type; therefore Class<MyNumber> should be the same as Class<Number>.

Again, I think it's more surprising that users could inadvertently build a nothing type without realizing and have all their type checks silently fail

I'd expect the failures to be loud, rather than silent! If you try to assign anything to Class<Foo?>, Pkl throws an exception. Also, I'd expect that any usage would also be caught statically (e.g. red squiggly lines in the editor during assignment). We can also add a warning diagnostic if you declare Class<Foo?> (or similar).

@HT154 HT154 force-pushed the class-no-erasure branch from acd6e0a to 6635dc3 Compare July 3, 2026 00:56
@HT154

HT154 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Updated this to unwrap typealiases during class calculation, so aliases (to aliases)+ to classes now behave as if the underlying class was used directly.

Also updated this to error at runtime rather than during ClassClassTypeNode init (or alias instantiation) with a hint that such type checks can never succeed. I think the errors are less helpful in cases where the Class type node or the type arg are behind aliases (since previously it relied on the same machinery as ReferenceTypeNode's constraint-in-referent checking). The only real difference other than error message content is that "bad" type annotations that never execute never raise errors. I can live with either approach, but I think the runtime errors aren't as good.

@HT154 HT154 force-pushed the class-no-erasure branch from fa43531 to 8148b52 Compare July 3, 2026 01:17
@HT154 HT154 force-pushed the class-no-erasure branch from 8148b52 to 9a2d11a Compare July 3, 2026 02:11
@HT154 HT154 force-pushed the class-no-erasure branch from 9a2d11a to e8626ff Compare July 3, 2026 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Class<T> is always type-erased to Class Incorrect constant expression analysis through Class<T>

2 participants